-
Notifications
You must be signed in to change notification settings - Fork 585
Update default settings for SameSite #1232
Conversation
|
@JunTaoLuo, |
| ExpireTimeSpan = TimeSpan.FromDays(14); | ||
| SlidingExpiration = true; | ||
| CookieSameSite = SameSiteMode.Strict; | ||
| CookieSameSite = SameSiteMode.Lax; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put a comment in indicating why we're using lax? Even just a link to the issue would be sufficient. Just something so that somebody doesn't come along 6 months later and decide to change it :)
Tratcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the OIDC & Social samples work.
| { | ||
| HttpOnly = true, | ||
| SameSite = SameSiteMode.Lax, | ||
| SameSite = SameSiteMode.Strict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, does that really work with the OIDC client when using response_mode=form_post? (in this case, the IdP returns an auto-submit HTML form pointing to the callback URL).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'm curious, what's the point of using a same-site policy for the correlation/anti-XSRF cookies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, Strict isn't going to work.
For consistency we're considering setting SameSite for all cookies that can work with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency is great and should always win, but be careful: RemoteAuthenticationHandler is a very generic base class that can be used to implement custom protocols (I personally wrote an OpenID 2.0 middleware leveraging it). That would be annoying to break scenarios that do fancy things with the callback dance... this same-site thingy tends to be really zealous 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, Strict isn't going to work.
@Tratcher I'm not sure Lax will work with response_mode=form_post since POST is not considered as a "safe" method by the specification.
@JunTaoLuo did Lax work for you?
| { | ||
| HttpOnly = true, | ||
| SameSite = SameSiteMode.Lax, | ||
| SameSite = SameSiteMode.Strict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark as above. Twitter - that implements OAuth1 - uses GET/302 responses to return the user back to the client app. If Strict doesn't work with the final application cookie due to the cross-origin 302 dance - as explained by @anurse - it will likely not work here.
|
Tested OIDCSample and OIDC.AzureAdSample. This is ready to merge |
4d0a986 to
cf99250
Compare
- Need Lax policy for social authentication - Need None policy for OIDC
cf99250 to
c523839
Compare
Addresses #1231 Need to test this with the auth samples before merging.